fix: buffer overflow in multipart body proc#3546
fix: buffer overflow in multipart body proc#3546airween wants to merge 5 commits intoowasp-modsecurity:v3/masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a potential out-of-bounds read/heap buffer overflow in multipart Content-Disposition parsing, based on an external bug report.
Changes:
- Adds an explicit
*(p+2) == '\0'guard before validating the second hex digit infilename*percent-escapes. - Avoids incrementing
ppast the string terminator when a quoted parameter value is missing a closing quote.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (*p == quote) { | ||
| p++; /* go over the quote at the end */ | ||
| } | ||
|
|
There was a problem hiding this comment.
The PR addresses out-of-bounds reads in Content-Disposition parsing; adding regression cases for (1) filename* values ending with an incomplete percent-escape (e.g. trailing %A) and (2) quoted values missing a terminating quote would help ensure this remains fixed (especially under ASAN/UBSAN).
| if (*p == quote) { | |
| p++; /* go over the quote at the end */ | |
| } | |
| if (*p == '\0') { | |
| /* missing terminating quote */ | |
| return -7; | |
| } | |
| p++; /* go over the quote at the end */ |
There was a problem hiding this comment.
I'm not sure this is a valid case, multipart header can contain urlencoded text, but the parser's task process it as is.
This suggestion removes that very important condition, I'm afraid that could make the parser wrong.
There was a problem hiding this comment.
You only get here if a opening quote was observed, so a closing quote is expected. If the closing quote is URL encoded, then that looks like an evasion attempt. If the opening quote is also URL encoded, we won't end up here anyway (I think), so I think it's valid to say that the quote is missing.
I don't think we should set invalid_quoting in this case, the semantics are different.
I don't understand what difference it makes for you whether the return value is -7 or -15. The previous suggestion would behave the same, just with a different return code.
There was a problem hiding this comment.
I think here we should split up the suggestions into two parts:
- handling url-encoded characters
- handling missing quote at the end
First item: not relevant, the parser does not handle the url encoded quotes at all, this is why I wrote that the suggestion from Copilot is not a valid case.
Second: the missing quote is handled correctly with this, I added a new test case; I checked this test without this modification, which was failed.
I don't understand what difference it makes for you whether the return value is -7 or -15. The previous suggestion would behave the same, just with a different return code.
I think this is why we shouldn't change anything here 😃.
| if (*p == quote) { | ||
| p++; /* go over the quote at the end */ | ||
| } | ||
|
|
There was a problem hiding this comment.
You only get here if a opening quote was observed, so a closing quote is expected. If the closing quote is URL encoded, then that looks like an evasion attempt. If the opening quote is also URL encoded, we won't end up here anyway (I think), so I think it's valid to say that the quote is missing.
I don't think we should set invalid_quoting in this case, the semantics are different.
I don't understand what difference it makes for you whether the return value is -7 or -15. The previous suggestion would behave the same, just with a different return code.
|
what
This PR fixes a possible heap buffer overflow in multipart body processor.
why
There is a bug report, received in email from @fumfel and his team. Also they provided this fix.
references
The original report:
other notes
After a review and a short discussion, @fumfel and his team confirmed that this issue can occur if the source code was built with ASAN flags.